Skip to content

[Wang Yihe] iP#243

Open
Yihe-Harry wants to merge 42 commits into
nus-cs2103-AY2021S2:masterfrom
Yihe-Harry:master
Open

[Wang Yihe] iP#243
Yihe-Harry wants to merge 42 commits into
nus-cs2103-AY2021S2:masterfrom
Yihe-Harry:master

Conversation

@Yihe-Harry
Copy link
Copy Markdown

No description provided.

@Yihe-Harry
Copy link
Copy Markdown
Author

[Wang Yihe] iP

Comment thread src/main/java/duke/TaskList.java Outdated
throw new DukeException("Missing component: due date");
}
String time = deadlineArr[1];
if (isDateFormat(time, "yyyy-mm-dd") || isDateFormat(time, "yyyy-m-dd") || isDateFormat(time, "yyyy-mm-d") || isDateFormat(time, "yyyy-m-d")) {
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps might want to add more spaces in front to indent this method to fit with the Java coding convention.

Comment thread src/main/java/duke/Ui.java Outdated

package duke.ui;

import duke.task.*;
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might be a better idea to import relevant files from the task folder instead of importing all?

Comment thread src/main/java/duke/Ui.java Outdated
printLine();
printMessage("Here are the matching tasks in your list");
for (int i = 0; i < task.size(); i++) {
int index = i + 1;
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like how you remember to add spaces around i + 1;

Comment thread src/main/java/duke/Ui.java Outdated
String[] input = temp.split(" ", 2);
Command command = getUserInputType(input[0]);
switch (command) {
case DEADLINE:
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"The explicit //Fallthrough comment should be included whenever there is a case statement without a break statement.", quoted from Java coding convection.

Comment thread src/main/java/duke/FileSaver.java Outdated

import duke.ui.Ui;
import duke.exception.DukeException;
import duke.task.*;
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Although all the three classes in task package are used, perhaps listing them out explicitly will comply with the coding standard better? 🤔

Comment thread src/main/java/duke/TaskList.java Outdated

public void add(String[] userInput, Ui ui) throws DukeException {
switch (userInput[0]) {
case "todo":
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

According to our coding standard, perhaps there should not be spaces before each "case"? 🤔

Copy link
Copy Markdown

@ljhgabe ljhgabe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work! LGTM except for some naming issues.

}

@Override
public String savedFormat() {
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps the naming here could be even more straightforward? e.g. formatInFile.

task.add(t);
}

public static boolean isDateFormat(String date) {
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good naming for method!

Comment thread src/main/java/duke/FileSaver.java Outdated
return true;
}

public void parseTask(TaskList task, String info) throws IndexOutOfBoundsException{
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it is a "TaskList", perhaps can rename the variable name to "tasks"? 🤔 (same applies to the rest methods)

return "[ ] " + this.description;
}
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps more Javadoc comments could be included? 🤔

User Guide 📣
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants